Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dummy node logic #187

Merged
merged 23 commits into from
Nov 23, 2024
Merged

Dummy node logic #187

merged 23 commits into from
Nov 23, 2024

Conversation

sumedhars
Copy link
Contributor

No description provided.

Copy link
Collaborator

@ntalluri ntalluri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was a quick review of the PR. I still need to test the PR locally

config/config.yaml Outdated Show resolved Hide resolved
input/dummy.txt Outdated Show resolved Hide resolved
input/dummy-1.txt Outdated Show resolved Hide resolved
spras/omicsintegrator1.py Outdated Show resolved Hide resolved
@ntalluri
Copy link
Collaborator

Can you also update the input/readme with information about the dummy file and dummy column in the prizes file

config/config.yaml Outdated Show resolved Hide resolved
input/dummy-1.txt Outdated Show resolved Hide resolved
@ntalluri
Copy link
Collaborator

If I try to use an updated dummy nodes file (dummy-1.txt) with the original sources, targets, and node-prizes files, I get this error:

Running docker containers
RuleException:
ValueError in file /Users/nehatalluri/Desktop/research/spras/Snakefile, line 159:
You are trying to merge on object and float64 columns. If you wish to proceed you should use pd.concat
  File "/Users/nehatalluri/Desktop/research/spras/Snakefile", line 159, in __rule_merge_input
  File "/Users/nehatalluri/Desktop/research/spras/spras/runner.py", line 42, in merge_input
  File "/Users/nehatalluri/Desktop/research/spras/spras/dataset.py", line 27, in __init__
  File "/Users/nehatalluri/Desktop/research/spras/spras/dataset.py", line 124, in load_files_from_dict
  File "/Users/nehatalluri/anaconda3/envs/spras/lib/python3.11/site-packages/pandas/core/frame.py", line 10093, in merge
  File "/Users/nehatalluri/anaconda3/envs/spras/lib/python3.11/site-packages/pandas/core/reshape/merge.py", line 110, in merge
  File "/Users/nehatalluri/anaconda3/envs/spras/lib/python3.11/site-packages/pandas/core/reshape/merge.py", line 707, in __init__
  File "/Users/nehatalluri/anaconda3/envs/spras/lib/python3.11/site-packages/pandas/core/reshape/merge.py", line 1340, in _maybe_coerce_merge_keys
[Fri Oct 11 15:43:19 2024]
Error in rule merge_input:
    jobid: 0
    input: input/node-prizes.txt, input/sources.txt, input/targets.txt, input/dummy-1.txt, input/network.txt
    output: output/dummy/data0-merged.pickle

this is what my config file looks like for the datasets

datasets:
    -
      # Labels can only contain letters, numbers, or underscores
      label: data0
      node_files: ["node-prizes.txt", "sources.txt", "targets.txt", "dummy-1.txt"]
      # DataLoader.py can currently only load a single edge file, which is the primary network
      edge_files: ["network.txt"]
      # Placeholder
      other_files: []
      # Relative path from the spras directory
      data_dir: "input"

this is what I updated dummy-1.txt to be:
A

@ntalluri
Copy link
Collaborator

ntalluri commented Oct 11, 2024

Could you also add tests to the OI1 testing sweep that test's the run function using the dummy nodes

There is a chance you will need to update the generate inputs test sweep as well (still need to think through if you really do).

@sumedhars
Copy link
Contributor Author

sumedhars commented Oct 18, 2024

If I try to use an updated dummy nodes file (dummy-1.txt) with the original sources, targets, and node-prizes files, I get this error:

@ntalluri
I think the error is because the file it titled dummy-1.txt instead of dummy.txt because wouldn't having the file title as dummy-1 cause the node.data_table dummy column to be called dummy-1 too? and in the oi1 code it looks for dummy in the node_table columns

@sumedhars sumedhars closed this Oct 18, 2024
@sumedhars sumedhars reopened this Oct 18, 2024
config/config.yaml Outdated Show resolved Hide resolved
config/config.yaml Outdated Show resolved Hide resolved
@ntalluri
Copy link
Collaborator

ntalluri commented Oct 21, 2024

Can you also run pre-commit run --all-files locally and push those changes. The 'Run pre-commit checks' test keeps failing.

@sumedhars
Copy link
Contributor Author

Can you also run pre-commit run --all-files locally and push those changes. The 'Run pre-commit checks' test keeps failing.
@ntalluri

Running that gave me this:

'[INFO] Initializing environment for https://github.com/charliermarsh/ruff-pre-commit.
[INFO] Installing environment for https://github.com/pre-commit/pre-commit-hooks.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
[INFO] Installing environment for https://github.com/charliermarsh/ruff-pre-commit.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
check yaml...............................................................Passed
check toml...............................................................Passed
trim trailing whitespace.................................................Failed

  • hook id: trailing-whitespace
  • exit code: 1
  • files were modified by this hook

Fixing spras/omicsintegrator1.py
Fixing config/config.yaml

ruff.....................................................................Failed

  • hook id: ruff
  • files were modified by this hook

Found 1 error (1 fixed, 0 remaining).'

what's the hook id: ruff refering to?

@ntalluri
Copy link
Collaborator

hook id: ruff

I think this is referring to this https://docs.astral.sh/ruff/, which is a linter and code formatter that pre-commit will use that was specified in the .pre-commit-config.yaml file

but that pre-commit message looks good due to this message Found 1 error (1 fixed, 0 remaining) which I always interpret that pre-commit was able to clean up the code.

Copy link
Collaborator

@agitter agitter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left a round of comments. I haven't run it locally yet.

Let's think of at least one test case we can add that will run Omics Integrator 1 with a dummy node file.

This will close #62 when merged.

input/README.md Outdated Show resolved Hide resolved
input/README.md Outdated Show resolved Hide resolved
input/README.md Outdated Show resolved Hide resolved
input/tps-egfr-prizes.txt Show resolved Hide resolved
input/node-prizes.txt Show resolved Hide resolved
spras/omicsintegrator1.py Outdated Show resolved Hide resolved
input/README.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@agitter agitter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested this with datasets that had dummy nodes and others that did not, and everything appeared to work. I also added simple test cases.

@agitter agitter merged commit 8fa5dae into Reed-CompBio:master Nov 23, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants